Fix: Linux CI failing due to too many arguments in ar command#37
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds configure(env) to install a subprocess-based SPAWN wrapper (shell=False, wait) with ar argument batching and forces ARFLAGS; generate(env) now calls configure. Separately, my_spawn's mySubProcess uses simplified Popen/wait and mySpawn batches long ar invocations into length-limited subprocess calls. ChangesLinux Build Spawn Wrapper
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Arctis-Fireblight
force-pushed
the
fix-linux-lines-too-long
branch
from
9d0747a to
71e44a1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/linux.py`:
- Around line 14-16: The file tools/linux.py was changed but not formatted,
causing CI to fail; run the project's formatter (e.g., ruff format) on
tools/linux.py (or the repo) to apply the blank-line and style fixes, verify
changes around the configure() function and the other noted sections (around
lines ~56-60 and ~97-99), and commit the formatted file so the formatting job
passes the CI.
- Line 57: The global env.Replace(ARFLAGS=["q"]) should be removed and the
ARFLAGS=["q"] setting applied only inside the batched ar handling path (the code
block that implements the batched/concatenated ar invocation), so that other
archive operations are unaffected; locate the batched path in tools/linux.py
(the block that builds the batched ar command) and set ARFLAGS or pass the "q"
flag locally there before invoking ar, leaving the global env.ARFLAGS unchanged.
- Around line 17-24: The code in mySubProcess currently reparses a command
string with shlex.split and passes that to subprocess.Popen, which can mangle
original argv boundaries; change mySubProcess to accept and pass an argv list
(or, if cmdline can be a list, detect and use it directly) and remove
shlex.split so you call subprocess.Popen(argv, env=env, ...) with the original
sequence; apply the same change to the other occurrence that mirrors lines 43-52
in this file (replace any re-parsing of a cmdline string with using the original
argv list or explicit shell=True if you truly need shell parsing).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| return rv | ||
|
|
||
| env["SPAWN"] = mySpawn | ||
| env.Replace(ARFLAGS=["q"]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "linux.py" --path "*/tools/*"Repository: Redot-Engine/redot-cpp
Length of output: 300
🏁 Script executed:
cat -n tools/linux.py | head -80Repository: Redot-Engine/redot-cpp
Length of output: 2998
🏁 Script executed:
rg -i "batch.*ar|ar.*batch" tools/linux.py -B 2 -A 2Repository: Redot-Engine/redot-cpp
Length of output: 308
🏁 Script executed:
wc -l tools/linux.pyRepository: Redot-Engine/redot-cpp
Length of output: 85
🏁 Script executed:
cat -n tools/linux.pyRepository: Redot-Engine/redot-cpp
Length of output: 3805
🏁 Script executed:
rg "use_llvm" tools/linux.py -B 3 -A 3Repository: Redot-Engine/redot-cpp
Length of output: 447
🏁 Script executed:
rg "ARFLAGS|ar\.command|\bAR\b" tools/linux.py -B 2 -A 2Repository: Redot-Engine/redot-cpp
Length of output: 127
🏁 Script executed:
git log --oneline tools/linux.py | head -20Repository: Redot-Engine/redot-cpp
Length of output: 136
🏁 Script executed:
git diff HEAD~5 tools/linux.pyRepository: Redot-Engine/redot-cpp
Length of output: 255
🏁 Script executed:
fd -t f "\.py$" tools/ | xargs rg -l "batch.*ar|ar.*batch" 2>/dev/nullRepository: Redot-Engine/redot-cpp
Length of output: 81
🏁 Script executed:
rg -i "batch.*ar|ar.*batch|archived.*batch" --type pyRepository: Redot-Engine/redot-cpp
Length of output: 123
🏁 Script executed:
rg "ar\.command|ARFLAGS" --type py -B 3 -A 3Repository: Redot-Engine/redot-cpp
Length of output: 906
🏁 Script executed:
find . -name "*.gn" -o -name "*.gni" | head -20Repository: Redot-Engine/redot-cpp
Length of output: 48
🏁 Script executed:
rg "ARFLAGS|ar_command|ar\.command" --type gn --type gni -B 2 -A 2 2>/dev/null | head -50Repository: Redot-Engine/redot-cpp
Length of output: 48
🏁 Script executed:
rg -i "batch" --type gn | head -20Repository: Redot-Engine/redot-cpp
Length of output: 215
🏁 Script executed:
cat tools/linux.pyRepository: Redot-Engine/redot-cpp
Length of output: 3105
🌐 Web query:
GNU ar command q flag behavior replace update
💡 Result:
In GNU ar, the q (quick append) flag is designed to add files to the end of an archive without checking for existing members with the same name [1][2][3]. Because it skips this check, it is faster than the r (replace) operation [1][4]. Key behaviors of the q flag include: 1. No Replacement: It does not replace existing members; it simply appends new files to the end of the archive [1][2]. 2. Placement: Modifiers such as a, b, or i, which are used to specify the position of new members, do not affect the q operation; new members are always placed at the end [1][3]. 3. Symbol Table: Historically, q did not update the archive's symbol table index to maintain speed [1][2]. However, because many systems expect the index to be current, GNU ar will rebuild the symbol table even when using q [2]. 4. Compatibility Note: GNU ar treats the command qs as a synonym for r, meaning it will replace existing files and append new ones [2][4][3]. 5. Incompatibility with u: The u (update) modifier, which only inserts files newer than those already in the archive, is not allowed with q because checking timestamps would negate the speed advantage of the quick append operation [1][4][3].
Citations:
- 1: https://sourceware.org/binutils/docs-2.20/binutils/ar-cmdline.html
- 2: https://man7.org/linux/man-pages/man1/ar.1.html
- 3: https://sourceware.org/binutils/docs/binutils.html
- 4: https://www.mankier.com/1/ar
🌐 Web query:
llvm-ar q flag quick append behavior duplicates
💡 Result:
The q (quick append) flag in llvm-ar is used to append files to the end of an archive without checking for or removing duplicates [1][2]. Because it does not perform any deduplication, it is generally faster than the r (replace) operation, which must scan the archive to identify and replace existing members with the same name [1][2]. If the specified archive does not exist, llvm-ar will create it [1][2].
Citations:
- 1: https://llvm.org/docs/CommandGuide/llvm-ar.html
- 2: https://rocm.docs.amd.com/projects/llvm-project/en/latest/LLVM/llvm/html/CommandGuide/llvm-ar.html
🏁 Script executed:
rg "use_llvm|ARFLAGS" tools/ -B 5 -A 5Repository: Redot-Engine/redot-cpp
Length of output: 7173
Fix global ARFLAGS setting.
The q flag applies globally to all ar invocations on Linux, but only the batched path (lines 44-50) requires special handling to work around command-line length limits. Both GNU ar and llvm-ar implement q as quick append without deduplication, so the concern about incompatible behavior across toolchains is unfounded. However, forcing q globally still affects all archive operations; if needed, apply it conditionally only within the batched path to avoid unintended side effects elsewhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/linux.py` at line 57, The global env.Replace(ARFLAGS=["q"]) should be
removed and the ARFLAGS=["q"] setting applied only inside the batched ar
handling path (the code block that implements the batched/concatenated ar
invocation), so that other archive operations are unaffected; locate the batched
path in tools/linux.py (the block that builds the batched ar command) and set
ARFLAGS or pass the "q" flag locally there before invoking ar, leaving the
global env.ARFLAGS unchanged.
Added logic to linux.py split long `.o` file lists into commands of 512 or less.
Arctis-Fireblight
force-pushed
the
fix-linux-lines-too-long
branch
from
71e44a1 to
db53da2
Compare
… commands. In `my_spawn.py`, the command line length cannot exceed 32000 characters on windows. The previous implementation would break down linking to one `.obj` file at a time which was extremely slow. The new batching logic instead will batch as many `.obj` files as possible into a single linking operation resulting in significantly faster build times when compiling using MinGW on Windows.
Arctis-Fireblight
force-pushed
the
fix-linux-lines-too-long
branch
from
916313e to
c18312f
Compare
…compiling with MinGW and on linux The lines removed were forcing Scons to effectively run single threaded.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/linux.py (1)
36-44: 💤 Low valueBatching strategy differs from
my_spawn.py.This file batches by argument count (510 per batch), while
tools/my_spawn.pybatches by command-line length (32000 chars). Linux'sARG_MAXis typically ~2MB, so the 512-argument limit seems overly conservative for most systems. Consider documenting why this specific threshold was chosen, or aligning the approach withmy_spawn.pyfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/linux.py` around lines 36 - 44, The batching uses a fixed 510-argument chunking in the block that constructs cmdline (vars: cmd, args, cmdline, loop over range(3, len(args), 510)) which differs from tools/my_spawn.py's length-based batching; update tools/linux.py to either document why 510/512 was chosen or replace the argument-count split with a command-line length-based splitter like my_spawn.py (use cumulative len(cmdline + " ".join(batch)) with a safe max (e.g., 32000 or configurable ARG_MAX) to decide batch boundaries), and keep the subprocess invocation using mySubProcess and the existing error-break logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tools/linux.py`:
- Around line 36-44: The batching uses a fixed 510-argument chunking in the
block that constructs cmdline (vars: cmd, args, cmdline, loop over range(3,
len(args), 510)) which differs from tools/my_spawn.py's length-based batching;
update tools/linux.py to either document why 510/512 was chosen or replace the
argument-count split with a command-line length-based splitter like my_spawn.py
(use cumulative len(cmdline + " ".join(batch)) with a safe max (e.g., 32000 or
configurable ARG_MAX) to decide batch boundaries), and keep the subprocess
invocation using mySubProcess and the existing error-break logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eead553b-e757-4e99-b9e7-b83f1a362d1b
📒 Files selected for processing (2)
tools/linux.pytools/my_spawn.py
This job takes well over an hour to run even with the recent performance boosts. Something is very clearly wrong with how Scons is interacting with MinGW since the CMake jobs complete within a few minutes. Given this is not a common configuration, we are disabling this workflow and will rely on the CMake version of this test to check MinGW compatibility instead.
1 participant
Added logic to linux.py split long
.ofile lists into commands of 512 or less.Summary by CodeRabbit